Architecture is exactly right — hfId stored on TimelineElement at parse time, carried through buildPatchTarget, and surfaced as the primary key when present. The four harvesting sites in timelineDOM.ts are symmetric and complete:

• createTimelineElementFromManifestClip (main path + the hostEl-resolved path at line 130)
• createImplicitTimelineLayersFromDOM
• parseTimelineFromDOM

The hfId-only fallback in buildPatchTarget is the right safety net for elements that have a stable data-hf-id but no #id. Tests cover all three parse sites plus the buildPatchTarget logic — solid.

P3 — minor: buildPatchTarget type signature could be narrowed.

The function signature uses an inline { domId?: string; hfId?: string; selector?: string; selectorIndex?: number } object type. Since TimelineElement already declares hfId, importing and using Pick<TimelineElement, "domId" | "hfId" | "selector" | "selectorIndex"> as the parameter type would tie the signature directly to the store type and catch future field renames. Not a blocker.

Producer failure artifacts (visual-failures.json, binary frames) are clearly pre-existing fixture noise that moved when the stack rebased — not caused by this PR's logic. The diff shows only PSNR value deltas on the same test names, not new test names appearing. Fine to carry.

✅ Approve.

Reviewed at 4731cde. Stack 3/3 — wires hfId on timeline path (+901/-644, but only ~78 lines of actual code; the rest is fixture-file churn). Code change is clean and consistent with the #1296/#1297 pattern: TimelineElement.hfId? field, three DOM-parse sites populate it from data-hf-id, and buildPatchTarget in useTimelineEditing includes hfId in all return paths with a new hfId-only fallback for elements that have a stable hfId but no domId. The new fallback is the right shape — preserves backward compat (domId path still works) while enabling hfId-only addressing for dynamically-generated DOM that doesn't have a stable id.

End-to-end check on Home's review angles:

• hfId presence/absence (angle #1): ✓ TimelineElement.hfId is optional; buildPatchTarget checks domId → hfId → selector in precedence.
• Server acceptance + type parity (angles #2, #3): ✓ Inherits from #1296/#1297.
• Cutover ordering (angle #4): ✓ Server-side R1 already in place, client-side now sends hfId on every timing/delete/split mutation.
• Backwards-compat (angle #6): ✓ Optional everywhere.
• R7 prior-stack interaction (angle #8): ✓ Consistent with PreviewAdapter's hit-test contract.

But one of Home's angles raised a real bug surface that this PR makes user-visible:

• Deletion/split correctness (angle #5): ❗ See concern #1 below.

Concerns

1. Split duplicates data-hf-id on the clone — hfId-as-primary-key contract breaks post-split

sourceMutation.ts:241-243:

const clone = el.cloneNode(true) as HTMLElement;
clone.setAttribute("id", newId);
clone.setAttribute("data-start", String(Math.round(splitTime * 1000) / 1000));
clone.setAttribute("data-duration", String(Math.round(secondDuration * 1000) / 1000));

cloneNode(true) deep-copies all attributes including data-hf-id. Only id, data-start, data-duration (and data-playback-start / data-media-start if present) are reassigned. So after split:

• Original: id="hero", data-hf-id="hf-abc"
• Clone: id="hero-2" (or whatever newId is), data-hf-id="hf-abc" ← duplicate

Once this PR lands, the studio's timeline operations send target.hfId="hf-abc" first. Server-side findByHfId finds BOTH elements, logs a multi-match warning (per sourceMutation.ts:54 from #1272), and picks the first one. User edits the wrong half of the split silently.

This is in sourceMutation.ts, not in this PR's diff, but #1299 activates the bug — before this PR, the timeline path was id-keyed, and split (which assigns a new id to the clone) kept the two halves distinguishable. After this PR makes hfId the primary key, the two halves collide.

Three fix options, in order of preference:

• (a) Mint a new hfId for the clone in splitElementInHtml:

  // After clone.cloneNode + id reset:
  clone.setAttribute("data-hf-id", mintHfId());   // new minted id

  Requires plumbing the hfId minter (which lives in #1270's ensureHfIds or the R7 stack) into splitElementInHtml. Right shape long-term — both halves are independently addressable.
• (b) Clear the clone's hfId so it gets backfilled on the next save:

  clone.removeAttribute("data-hf-id");

  Cheaper — the clone's hfId is undefined until R7's write-back path re-mints it. In the meantime, the clone falls back to id/selector addressing, which works because the clone has a fresh id. The downside: a window where hfId-based references to the clone don't work (but no one has a reference to it yet, since it's brand new).
• (c) Keep the original's hfId on whichever half is "the original" (typically the first half) and clear the clone's. Same as (b) but explicit.

(b) and (c) are the right answer for this PR cycle if (a) requires too much plumbing; (a) is the clean long-term shape.

The fix lives in packages/core/src/studio-api/helpers/sourceMutation.ts and is outside this PR's stated scope, but I'd recommend bundling it here because the bug becomes user-visible the moment #1299 lands.

2. data-hf-id empty-string handling — now FIVE sites with the same issue

The four new reads in timelineDOM.ts (lines 76, 132, 192, 269):

hfId = hostEl.getAttribute("data-hf-id") ?? undefined;
entry.hfId = hostEl.getAttribute("data-hf-id") ?? undefined;
hfId: child.getAttribute("data-hf-id") ?? undefined,
hfId: el.getAttribute("data-hf-id") ?? undefined,

…plus the single read in domEditingLayers.ts from #1296 and the read in useDomEditCommits.ts:556 from #1297 = 5 read sites total across the stack. All with the same ?? undefined shape that doesn't handle the empty-string case (per my #1296 concern #1 and #1297 concern #2).

A data-hf-id="" element produces selection.hfId === "", which becomes [data-hf-id=""] selector at the server side. Likely fine in practice (no one creates empty hf-ids deliberately), but as the read sites proliferate, the chance of a fixture / migration / test setup producing an empty value grows.

Concrete:

// packages/studio/src/components/editor/domEditingLayers.ts
export function readHfId(element: Element): string | undefined {
  return element.getAttribute("data-hf-id")?.trim() || undefined;
}

Then replace all 5 sites. Cheap, centralizes the normalization, and gives a single place to add future logic (e.g., reject malformed hf-ids, log telemetry).

I flagged this as a nit in the #1296/#1297 reviews. At 5 sites in 3 PRs in one stack, it's worth landing the helper here.

3. failures/ fixture-file diffs (same family as #1297 concern #1)

This PR touches 7 failures/visual-failures.json files (~823 lines of JSON diff across chat, iframe-render-compat, render-symlinked-assets, vfr-screen-recording) plus many binary .png files in their failures/frames/ directories.

Same question as my #1297 concern #1: the failures/ directory naming implies "captured on test failure" artifacts. CI status confirms all regression-shards pass on this PR, which means these aren't load-bearing golden files. Two scenarios:

• (a) Test framework writes these always, not just on failure. They're snapshots that get regenerated, and committing them keeps the diff clean for future PRs.
• (b) They were committed accidentally — the author ran tests locally, hit a transient failure, captured the artifacts, and added them to the PR.

Looking at the file structure (PNG actual_*.png and expected_*.png side-by-side in the same directory), this looks like (a) — the framework captures both for visual diff debugging. But the question stands: should these be in a code PR?

Two reasonable resolutions:

• (a-confirm) If the framework regenerates on every run, add a .gitignore rule for failures/ and skip committing them. The diff for this PR shrinks 90%.
• (b-revert) If they were committed accidentally, revert them and let CI rebuild on next run.

The actual code change in #1299 (78 lines across 4 files) is what matters; the fixture churn is making the review harder than it needs to be.

Nits

• useTimelineEditing.ts:51-58 — buildPatchTarget precedence is now domId → hfId → selector. The domId path also includes hfId (so the server can use whichever it prefers). The hfId-only path omits id (because there's no domId to send). The selector-only path omits both. Worth a one-line comment on the function describing the precedence so the next reader doesn't have to trace three return paths:

  // Patch target precedence: domId (with hfId companion) → hfId-only → selector.
  // Server-side findTargetElement prefers hfId when both are present; the domId
  // companion lets the legacy id-keyed code path still find the element if the
  // server falls back from hfId lookup.

• timelineDOM.test.ts:11-15 — makeDoc helper is good. The third test (createImplicitTimelineLayersFromDOM) uses the same helper. Worth one more test for createTimelineElementFromManifestClip (the most-complex path with the host-resolution + composition-host re-read) to round out the harness.
• playerStore.ts:24-25 — hfId is now part of the TimelineElement Zustand state. If there's any persistence (localStorage, IndexedDB), old stored elements won't have hfId on rehydration. Probably non-issue (timeline state is derived from the DOM on every load), but worth confirming.

Questions

• Split-then-target bug — is the fix in scope for this PR or a follow-up? Concern #1.
• failures/ fixture-file commits — intentional (per concern #3) or accidental? Affects how the reviewer should treat the diff.
• Telemetry for primary-vs-secondary key usage — same question as #1297 nit. As hfId becomes the primary, tagging which patches hit hfId vs id vs selector would let the team see adoption.

What I didn't verify

• The full integration through the timeline persistence path (persistTimelineEdit, handleTimelineElementDelete, handleTimelineElementSplit) — assumed they all call buildPatchTarget(element) and the new hfId-bearing return propagates through.
• That the new hfId-only fallback path actually fires in practice (i.e., are there real TimelineElements without domId? Implicit layers might be — the test at L43-54 confirms createImplicitTimelineLayersFromDOM does populate hfId on layers).
• The PNG binary diffs in failures/frames/ — git shows them as 0/0 line changes (binary). Couldn't sample without dumping to disk.

Clean, focused wiring change. The split-time hfId collision is the only real concern; the empty-string normalization is the obvious cleanup; the fixture diff is the procedural question.

— Review by Rames D Jusso

Re-reviewing at 1a4f4e5.

The new commit is a solid response. Running down the prior concerns:

✅ Fixed

• readHfId helper extracted (getAttribute("data-hf-id")?.trim() || undefined) — normalizes null, empty-string, and whitespace-only in one place; 4 unit tests cover all cases.
• resolveDomEditSelection and useDomEditCommits.ts:554 both use readHfId — the duplicate ?? undefined sites in the DOM-edit path are gone.
• resolveDomEditCapabilities guard widened to (!selector && !hfId) — correct: elements addressable only by hfId can now be selected and styled, not silently rejected. Good catch that wasn't in the prior review.
• getDomEditTargetKey widened to include hfId — prevents false cache collisions between elements that share a selector but differ by hfId. Also a new fix, right call.
• PatchTarget comment added — documents the server-side validation boundary cleanly.
• fallow-ignore suppressions on backward-compat re-exports — unblocks the pre-commit hook.

One inconsistency (nit, not blocking)

timelineDOM.ts still reads getAttribute("data-hf-id") || undefined at 4 sites rather than importing readHfId. This is likely intentional — player/lib shouldn't import from components/editor. The || form handles empty-string (unlike the original ??) so it's functionally correct; it only misses the .trim() for whitespace-only IDs. Accept as-is unless the normalization logic ever diverges.

Still unaddressed: split/clone hfId collision (Rames concern #1)

splitElementInHtml in sourceMutation.ts does cloneNode(true), which copies data-hf-id onto the clone. After this stack lands, buildPatchTarget sends hfId as the primary key — findByHfId will hit two elements with the same hf-id, log a multi-match warning, and silently patch the wrong half of the split. The fix is a one-liner in sourceMutation.ts:

// After cloneNode + id/timing resets:
clone.removeAttribute("data-hf-id");   // prevent duplicate-hfId collision

This bug doesn't exist before this stack (hfId was never sent) but becomes user-visible the moment it lands. Recommend bundling here rather than a follow-up — the surface-area cost is one line in one file.

Whether that's a blocker or a fast-follow is the author's call, but Rames and I both flag it as the highest-risk item in the stack.

Re-reviewed at 1a4f4e5. Round-2 delta is small and clean: 6 files, +65/-21. Diff scope is 4731cde → 1a4f4e5.

What landed since round-1

✅ readHfId helper extracted at domEditingLayers.ts:563-565 — exactly the shape I proposed:

export function readHfId(element: Element): string | undefined {
  return element.getAttribute("data-hf-id")?.trim() || undefined;
}

Re-exported via domEditing.ts:38 for cross-module use. Test coverage added (domEditingLayers.test.ts:31-54) hits all four cases: present, absent, empty-string, whitespace-only. Good.

✅ Editor path now uses the helper — 2 sites: resolveDomEditSelection at domEditingLayers.ts:325, 373 and useDomEditCommits batch path at line 557.

✅ hfId plumbed deeper than I asked — bonus thoroughness:

• resolveDomEditCapabilities (line 175-186): hfId now sufficient to enable capabilities even without selector ((!args.selector && !args.hfId))
• probeSourceElement (line 293): sends hfId on existence check
• getDomEditTargetKey (line 542-549): hfId now part of cache key — prevents cache collision when two elements share an id/selector but have distinct hfIds
• DomEditLayerItem (line 470): carries hfId for layer panel

All the right places.

✅ sourcePatcher.ts:95-96 comment documents that runtime hfId validation lives in findTagByTarget → execDataAttrPattern — answers one of my round-1 questions about validation location. 👍

Concerns

1. Split-clone hf-id duplication — still present, still activated by this PR

packages/core/src/studio-api/helpers/sourceMutation.ts:276:

const clone = el.cloneNode(true) as HTMLElement;
clone.setAttribute("id", newId);
clone.setAttribute("data-start", ...);
clone.setAttribute("data-duration", ...);
// data-hf-id NOT reassigned → both halves share the same hf-id

This was concern #1 in round-1 and is the highest-stakes item in the stack. It's not in this round's delta. Three reasonable explanations:

• (a) Scoped as a follow-up PR — fine, but the bug becomes user-visible the moment #1299 lands, so the follow-up should be queued before merge.
• (b) Decided not in scope — also fine if there's a deliberate plan (e.g., wait for R7 T6 to handle re-minting).
• (c) Slipped — let me re-raise: post-merge, findByHfId on either half of a split returns BOTH elements, logs a multi-match warning (sourceMutation.ts:54), and silently picks the first one. User edits the wrong half. Repro: split a clip, then drag the second half — server-side patch hits the first half instead.

Could you confirm which of (a)/(b)/(c) is the intent? If (a) or (b), no action here; if it's still open, the one-liner fix is clone.removeAttribute("data-hf-id") post-clone (lets R7's ensureHfIds backfill on next save).

2. Timeline path bypasses readHfId — inconsistency by layering

The 4 reads at timelineDOM.ts:85, 140, 201, 277 use inline getAttribute("data-hf-id") || undefined instead of readHfId(el):

hfId = hostEl.getAttribute("data-hf-id") || undefined;

I assume this is intentional — timelineDOM.ts lives under player/lib/ and readHfId is exported from components/editor/ (player shouldn't depend on editor). If so, that's a fair layering call.

Two ways to tighten:

• (a) Move readHfId to a shared location like packages/studio/src/utils/hfId.ts (or alongside sourcePatcher.ts since both touch the same DOM contract). Lets both player + editor import it without crossing layers.
• (b) Leave as-is + add a one-line comment at the top of the 4 timelineDOM sites explaining why this isn't the helper.

Functional difference between || undefined and readHfId is whitespace-only values (" "): the inline form keeps them, the helper trims to undefined. Theoretical edge case (no one creates data-hf-id=" "), but inconsistency means the player and editor handle the same DOM attribute differently. (a) gets my vote; (b) is fine if the layering is a hard rule.

3. One more raw getAttribute("data-hf-id") site in the coalesce key

useDomEditCommits.ts:537:

const coalesceKey = `z-reorder:${entries.map((e) => e.id ?? e.selector ?? e.element.getAttribute("data-hf-id") ?? "el").join(":")}`;

Falls through to "el" on null, so empty-string/whitespace would pass into the key as "" — non-load-bearing (just affects coalesce grouping), but for consistency this should also be ... ?? readHfId(e.element) ?? "el" so the cache key normalizes the same way as the rest of the path.

4. Round-1 concern #3 (fixture diffs) — not addressed, but I'll soften my framing

72 files / +2076/-706 in packages/producer/tests/ is unchanged from round-1. Looking more carefully this time, the diffs split into two kinds:

• chat/failures/frames/*.png + chat/failures/visual-failures.json — these really look like captured-on-failure artifacts (the failures/ dir name + actual_* / expected_* PNG pairing).
• distributed/three-boundary*/{src,output,meta.json}/* — these look like legitimate fixture regen for an unrelated distributed-rendering test.

So round-1 me overgeneralized — the distributed/* changes are plausibly intentional. I'm walking back from "revert all of these" to "the chat/failures/* PNG set still looks accidental." If you'd rather kick that down to a separate cleanup PR (along with .gitignoreing failures/), totally fine.

Nits

• timelineDOM.ts:30-43 — the new // fallow-ignore-next-line unused-exports comments scattered above each re-export are noisy. If the linter complains about unused-exports on intentional re-exports, a single block-level /* eslint-disable unused-exports */ covering the whole export { ... } block would be cleaner than 6 inline comments.
• useTimelineEditing.ts:48-69 — buildPatchTarget still has the three-branch precedence (domId → hfId → selector) without a comment. My round-1 nit on this didn't get picked up, but it's tiny enough that I won't re-press unless you want it.

Questions

• Split bug status — follow-up PR queued, scoped out, or slipped? (Concern #1.)
• Layering on readHfId — is player/lib/ → components/editor/ a hard layering rule? Answers whether I should push for moving the helper to utils/. (Concern #2.)

What I verified this round

• git log 4731cde..1a4f4e5 — single commit refactor(studio): extract readHfId helper...
• All 5 originally-flagged read sites now either use readHfId or || undefined (verified by git grep getAttribute\(.data-hf-id.\) on pr-1299-v2).
• readHfId test coverage — 4 cases per domEditingLayers.test.ts:31-54.
• Helper implementation — matches my round-1 proposal byte-for-byte.

What I didn't verify

• Whether useTimelineEditing.ts's buildPatchTarget (the timeline-side patch builder) actually receives populated hfId in real flows — implicit in CI's regression-shards passing.
• The 72-file fixture diff in detail (sampled file names only).

Clean follow-up to round-1. Helper + tests + extra plumbing show care. Split bug is the one remaining blocker-adjacent item — once that's resolved (this PR or follow-up), this stack is shippable.

— Review by Rames D Jusso

Force-push confirmed clean — ~900 lines of fixture churn gone, only 11 files of actual code remain.

The split/clone fix (clone.removeAttribute("data-hf-id") in splitElementInHtml) is exactly right, and the test (splitElementInHtml — hfId clone isolation) locks the contract: exactly one occurrence of the original hf-id after split. That was the highest-risk item in the stack.

Stack is good end-to-end: every Studio edit (drag, resize, style, timeline timing, delete, split) now sends target.hfId to the server, the server's hfId-first lookup branches in sourceMutation.ts and sourcePatcher.ts are live, and splits produce uniquely-addressable halves.

✅ Approve.

Re-reviewed at 84616b7b. Delta is two code changes plus the fixture cleanup:

✅ Split-clone data-hf-id bug FIXED — sourceMutation.ts:278: clone.removeAttribute("data-hf-id"). Option (b) from round-1 concern #1, taken verbatim — the clone falls through to id/selector addressing until R7's write-back path re-mints on next save. Test added at sourceMutation.test.ts:460-468 checks (html.match(/data-hf-id="hf-abc123"/g) ?? []).length === 1 post-split. Clean, minimal, and verifies the contract.

✅ Fixture artifacts dropped — 67 files / -1736 lines from failures/* paths across chat/, style-2-prod/, style-10-prod/, vfr-screen-recording/. Confirms these were locally committed by accident, as you flagged on Slack.

The remaining nits from round-2 are unchanged and all non-blocking — timeline path bypasses readHfId (intentional layering), one raw getAttribute in useDomEditCommits.ts:537's coalesce key (non-load-bearing fallback), missing buildPatchTarget precedence comment (cosmetic). Happy to leave any/all of those to a follow-up.

No concerns from my side. R7 T5 stack is ready from where I sit pending CI.

— Review by Rames D Jusso